Fix CI test failures in feature/capgen, address pylint warnings by climbfuji · Pull Request #412 · NCAR/ccpp-framework

climbfuji · 2021-11-02T20:20:50Z

Description

This PR fixes the CI test failures reported in #411.

It also bumps up the pylint rating for test/unit_tests/test_metadata_table.py from below 9 to 10/10 (yay) by removing the unused variable tables in many places.

Caveats

When running

PYTHONPATH=$(pwd)/scripts:$(pwd)/scripts/parse_tools pytest

locally (this is what the GitHub workflow/CI test does), I still get several warnings:

dom.heinzeller@heinzeller-lt:~/work/ccpp-framework/ccpp-framework-feature-capgen-fix-ci-20211102 [gccgfortran-py3-hpc-stack-1.2.0]>  PYTHONPATH=$(pwd)/scripts:$(pwd)/scripts/parse_tools pytest
....                                                                                                                                                                                                                                               [100%]
==================================================================================================================== warnings summary ====================================================================================================================
scripts/state_machine.py:165
  /Users/dom.heinzeller/work/ccpp-framework/ccpp-framework-feature-capgen-fix-ci-20211102/scripts/state_machine.py:165: DeprecationWarning: Flags not at the start of the expression '(?:(?i)timestep_init' (truncated)
    regex = re.compile(value[2] + r"$")

scripts/state_machine.py:166
scripts/state_machine.py:166
scripts/state_machine.py:166
scripts/state_machine.py:166
scripts/state_machine.py:166
  /Users/dom.heinzeller/work/ccpp-framework/ccpp-framework-feature-capgen-fix-ci-20211102/scripts/state_machine.py:166: DeprecationWarning: Flags not at the start of the expression '([A-Za-z][A-Za-z0-9_' (truncated)
    function = re.compile(FORTRAN_ID + r"_(" + value[2] + r")$")

scripts/state_machine.py:165
  /Users/dom.heinzeller/work/ccpp-framework/ccpp-framework-feature-capgen-fix-ci-20211102/scripts/state_machine.py:165: DeprecationWarning: Flags not at the start of the expression '(?:(?i)timestep_fina' (truncated)
    regex = re.compile(value[2] + r"$")

scripts/state_machine.py:165
  /Users/dom.heinzeller/work/ccpp-framework/ccpp-framework-feature-capgen-fix-ci-20211102/scripts/state_machine.py:165: DeprecationWarning: Flags not at the start of the expression '(?:(?i)init(?:ial(?:' (truncated)
    regex = re.compile(value[2] + r"$")

scripts/state_machine.py:165
  /Users/dom.heinzeller/work/ccpp-framework/ccpp-framework-feature-capgen-fix-ci-20211102/scripts/state_machine.py:165: DeprecationWarning: Flags not at the start of the expression '(?:(?i)final(?:ize)?' (truncated)
    regex = re.compile(value[2] + r"$")

scripts/state_machine.py:165
  /Users/dom.heinzeller/work/ccpp-framework/ccpp-framework-feature-capgen-fix-ci-20211102/scripts/state_machine.py:165: DeprecationWarning: Flags not at the start of the expression '(?:(?i)run)$'
    regex = re.compile(value[2] + r"$")

-- Docs: https://docs.pytest.org/en/latest/warnings.html
4 passed, 10 warnings in 0.08s

…t rating

gold2718

Wondering why whitespace changes need to be in test_metadata_table.py

gold2718 · 2021-11-03T00:45:58Z

        with self.assertRaises(Exception) as context:
-            tables = parse_metadata_file(filename, known_ddts,
-                                         self._DUMMY_RUN_ENV)
+            parse_metadata_file(filename, known_ddts, self._DUMMY_RUN_ENV)


What is the point of all these whitespace changes?

Well, now the entire call fits in one line below 80 characters and thus easily on a terminal screen. I'll change it back ...

…data_table.py

gold2718

The issue before was not the whitespace change but why you are choosing to add unrelated changes to a bugfix PR. To quote a developer I know:

I think we should try to be good stewards of GitHub code change practices and create smaller PRs that contain logically related changes but nothing else / not much else otherwise going forward.

Co-authored-by: goldy <github@goldy.online>

…code changes, and add _

climbfuji · 2021-11-03T22:50:54Z

The issue before was not the whitespace change but why you are choosing to add unrelated changes to a bugfix PR. To quote a developer I know:

I think we should try to be good stewards of GitHub code change practices and create smaller PRs that contain logically related changes but nothing else / not much else otherwise going forward.

Except that there is a difference between removing a newline when changes that are bug fixes do shorten the lines so that they fit within reasonable limits or othewise require adjusting the indentation for the second line (and all this on 19 lines in a single file), and thousands of lines of code changes that are unrelated in one PR ...

At any rate, I committed your suggestion and changed all other occurrences to match it. Note that your suggested change also removed the newline (45415b5).

gold2718 · 2021-11-03T23:57:54Z

Except that there is a difference between removing a newline when changes that are bug fixes do shorten the lines so that they fit within reasonable limits ...

My issue is that unless we have a definition of "reasonable limits", we will keep having this problem.

gold2718

Still not sure why test_metadata_table.py needed any modification to fix the CI bug in test_metadata_parser.py

climbfuji added 3 commits November 2, 2021 14:03

Update test/unit_tests/test_metadata_table.py to get to a 10/10 pylin…

d7b5efa

…t rating

Fix CI test failures in tests/test_metadata_parser.py

e22dbfe

Remove executable flag from test/unit_tests/test_metadata_table.py

84437b9

climbfuji marked this pull request as ready for review November 2, 2021 20:21

climbfuji requested a review from gold2718 as a code owner November 2, 2021 20:21

gold2718 reviewed Nov 3, 2021

View reviewed changes

Turn one liners back into two line calls in test/unit_tests/test_meta…

56cc421

…data_table.py

climbfuji requested a review from gold2718 November 3, 2021 22:29

gold2718 reviewed Nov 3, 2021

View reviewed changes

Comment thread test/unit_tests/test_metadata_table.py Outdated

climbfuji and others added 2 commits November 3, 2021 16:35

Update test/unit_tests/test_metadata_table.py

45415b5

Co-authored-by: goldy <github@goldy.online>

Changed format back to what I had in the beginning, as per suggested …

20c72b9

…code changes, and add _

gold2718 approved these changes Nov 3, 2021

View reviewed changes

climbfuji merged commit 105d8b5 into NCAR:feature/capgen Nov 4, 2021

climbfuji mentioned this pull request Nov 4, 2021

feature/capgen: CI tests failing after latest merge (11/01) #411

Closed

climbfuji deleted the fix_ci_test_failures_feature_capgen_20211102 branch June 27, 2022 03:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix CI test failures in feature/capgen, address pylint warnings#412

Fix CI test failures in feature/capgen, address pylint warnings#412
climbfuji merged 6 commits into
NCAR:feature/capgenfrom
climbfuji:fix_ci_test_failures_feature_capgen_20211102

climbfuji commented Nov 2, 2021 •

edited

Loading

Uh oh!

gold2718 left a comment

Uh oh!

gold2718 Nov 3, 2021

Uh oh!

climbfuji Nov 3, 2021

Uh oh!

gold2718 left a comment

Uh oh!

Uh oh!

climbfuji commented Nov 3, 2021

Uh oh!

gold2718 commented Nov 3, 2021

Uh oh!

gold2718 left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

climbfuji commented Nov 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Caveats

Uh oh!

gold2718 left a comment

Choose a reason for hiding this comment

Uh oh!

gold2718 Nov 3, 2021

Choose a reason for hiding this comment

Uh oh!

climbfuji Nov 3, 2021

Choose a reason for hiding this comment

Uh oh!

gold2718 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

climbfuji commented Nov 3, 2021

Uh oh!

gold2718 commented Nov 3, 2021

Uh oh!

gold2718 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

climbfuji commented Nov 2, 2021 •

edited

Loading